src/mcp_agent/cli/cloud/commands/servers/delete/main.py (1)

38-66: Check permissions before prompting; fail fast.

Move the permission check above the confirmation prompt to avoid asking users to confirm actions they cannot perform.

     server_name = get_server_name(server)
     server_id = get_server_id(server)
 
-    if not force:
+    # Check permissions before any prompt
+    if isinstance(server, MCPApp):
+        can_delete = run_async(client.can_delete_app(server_id))
+    else:
+        can_delete = run_async(client.can_delete_app_configuration(server_id))
+    if not can_delete:
+        raise CLIError(
+            f"You do not have permission to delete this {server_type.lower()}. "
+            f"You can only delete servers that you created."
+        )
+
+    if not force:
         console.print(
             Panel(
                 f"Name: [cyan]{server_name}[/cyan]\n"
                 f"Type: [cyan]{server_type}[/cyan]\n"
                 f"ID: [cyan]{server_id}[/cyan]\n\n"
                 f"[bold red]⚠️  This action cannot be undone![/bold red]",
                 title="Server to Delete",
                 border_style="red",
                 expand=False,
             )
         )
         
         confirm = typer.confirm(f"\nAre you sure you want to delete this {server_type.lower()}?")
         if not confirm:
             print_info("Deletion cancelled.")
             return
-
-    if isinstance(server, MCPApp):
-        can_delete = run_async(client.can_delete_app(server_id))
-    else:
-        can_delete = run_async(client.can_delete_app_configuration(server_id))
-        
-    if not can_delete:
-        raise CLIError(
-            f"You do not have permission to delete this {server_type.lower()}. "
-            f"You can only delete servers that you created."
-        )

src/mcp_agent/cli/core/utils.py (1)

29-41: Tighten identifier parsing (avoid accidental misclassification)

Current fallback treats any string as an app_id. Consider stricter validation (prefix + simple pattern) to reduce surprises. Example:

 def parse_app_identifier(identifier: str) -> Tuple[Optional[str], Optional[str], Optional[str]]:
     """Parse app identifier to extract app ID, config ID, and server URL."""
-    
+    identifier = identifier.strip()
     if identifier.startswith(('http://', 'https://')):
         return None, None, identifier
     
     if identifier.startswith('apcnf_'):
         return None, identifier, None
     
     if identifier.startswith('app_'):
         return identifier, None, None
     
     return identifier, None, None

src/mcp_agent/cli/cloud/commands/servers/describe/main.py (5)

21-30: Rename CLI option var to avoid shadowing built-in and keep usage consistent

Rename parameter to output_format and update call sites.

-@handle_server_api_errors 
-def describe_server(
-    id_or_url: str = typer.Argument(..., help="Server ID or URL to describe"),
-    format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"),
-) -> None:
+@handle_server_api_errors 
+def describe_server(
+    id_or_url: str = typer.Argument(..., help="Server ID or URL to describe"),
+    output_format: str = typer.Option("text", "--format", help="Output format (text|json|yaml)"),
+) -> None:
     """Describe a specific MCP Server."""
-    validate_output_format(format)
+    validate_output_format(output_format)
     client = setup_authenticated_client()
     server = resolve_server(client, id_or_url)
-    print_server_description(server, format)
+    print_server_description(server, output_format)

---

35-38: De-duplicate format validation logic

Reuse the shared validator instead of a local list.

-    valid_formats = ["text", "json", "yaml"]
-    if output_format not in valid_formats:
-        raise CLIError(f"Invalid format '{output_format}'. Valid options are: {', '.join(valid_formats)}")
+    validate_output_format(output_format)

---

62-69: Normalize name and URL fields in dict output

Ensure human-friendly defaults.

-        server_name = server.name
+        server_name = server.name or "Unnamed"
@@
-    server_url = server_info.serverUrl if server_info else None
+    server_url = (server_info.serverUrl if server_info else None) or None

Also applies to: 81-97

---

104-111: Text renderer: default name and URL for better UX

Avoid printing “None”.

-        server_name = server.name
+        server_name = server.name or "Unnamed"
@@
-    if server_info:
+    if server_info:
         status_text = _server_status_text(server_info.status)
-        server_url = server_info.serverUrl
+        server_url = server_info.serverUrl or "N/A"

Also applies to: 119-131, 136-138

---

157-164: Deduplicate status-to-emoji helper across commands

This duplicates logic in servers/list. Consider centralizing a single helper in servers/utils to avoid drift.

src/mcp_agent/cli/cloud/commands/servers/utils.py (2)

101-103: Remove no-op replace in error wrapper

The second replace does nothing.

-            func_name = func.__name__.replace('_', ' ').replace('server', 'server')
+            func_name = func.__name__.replace('_', ' ')

---

48-50: Consider exposing valid formats once to avoid drift

Move the formats list to a module-level constant and reuse in describe/list/delete for consistency.

src/mcp_agent/cli/cloud/commands/servers/list/main.py (6)

23-27: Avoid shadowing built-ins (filter, format) and validate limit.

Rename params to reduce confusion; also add a min bound for limit.

-def list_servers(
-    limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"),
-    filter: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"),
-    sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"),
-    format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"),
-) -> None:
+def list_servers(
+    limit: Optional[int] = typer.Option(None, "--limit", min=1, help="Maximum number of results to return"),
+    filter_expr: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"),
+    sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"),
+    output_format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"),
+) -> None:

-    validate_output_format(format)
+    validate_output_format(output_format)

-    filtered_deployed = _apply_filter(list_apps_res.apps, filter) if filter else list_apps_res.apps
-    filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter) if filter else list_app_configs_res.appConfigurations
+    filtered_deployed = _apply_filter(list_apps_res.apps, filter_expr) if filter_expr else list_apps_res.apps
+    filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter_expr) if filter_expr else list_app_configs_res.appConfigurations

-    if format == "json":
+    if output_format == "json":
         _print_servers_json(sorted_deployed, sorted_configured)
-    elif format == "yaml":
+    elif output_format == "yaml":
         _print_servers_yaml(sorted_deployed, sorted_configured)
     else:
-        _print_servers_text(sorted_deployed, sorted_configured, filter, sort_by)
+        _print_servers_text(sorted_deployed, sorted_configured, filter_expr, sort_by)

Also applies to: 43-43, 58-69

---

89-93: Prefer explicit type checks for configurations.

Use isinstance(..., MCPAppConfiguration) instead of attribute probing.

-            elif hasattr(server, 'app'):  # MCPAppConfiguration
+            elif isinstance(server, MCPAppConfiguration):  # MCPAppConfiguration
                 name = server.app.name if server.app else ""
                 description = server.app.description if server.app else ""
                 status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE"

-            elif hasattr(server, 'app'):  # MCPAppConfiguration
+            elif isinstance(server, MCPAppConfiguration):  # MCPAppConfiguration
                 name = server.app.name if server.app else ""
                 created_at = server.createdAt
                 status = server.appServerInfo.status if server.appServerInfo else "APP_SERVER_STATUS_OFFLINE"

Also applies to: 134-138

---

179-179: Use Rich’s rule instead of a fixed 80-char line.

Adapts to terminal width automatically.

-    console.print("\n" + "─" * 80 + "\n")
+    console.rule()

---

218-218: Safer YAML emission.

Prefer safe_dump and preserve key order.

-    print(yaml.dump(output, default_flow_style=False))
+    print(yaml.safe_dump(output, default_flow_style=False, sort_keys=False))

---

49-53: Consider pagination to honor large result sets.

Currently only the first page (max_results) is fetched; iterate page tokens to collect all results when limit is None or above a page.

Would you like a follow-up patch adding paginated fetch for both APIs?

---

328-335: Deduplicate status rendering.

_server_status_text duplicates logic from servers/describe/main.py. Move a single implementation to servers/utils.py and reuse.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/mcp_agent/cli/cloud/commands/servers/utils.py (1)

89-105: Polish error messages: Title-case 'server' in function names

Matches prior feedback; makes messages like “Error in List Servers”.

-            func_name = func.__name__.replace('_', ' ')
+            func_name = func.__name__.replace('_', ' ').replace('server', 'Server')

src/mcp_agent/cli/cloud/commands/servers/list/main.py (3)

18-18: Fix created-date sorting: normalize tz and use epoch seconds

Avoids TypeError when mixing tz-aware/naive datetimes; sorts reliably.

-from datetime import datetime
+from datetime import datetime, timezone
@@
-        elif sort_field_lower in ['created', 'created_at', 'date']:
-            return created_at or datetime.min.replace(tzinfo=created_at.tzinfo if created_at else None)
+        elif sort_field_lower in ['created', 'created_at', 'date']:
+            if created_at:
+                # Treat naive as UTC; normalize aware to UTC; compare by epoch seconds
+                if created_at.tzinfo is None:
+                    created_at = created_at.replace(tzinfo=timezone.utc)
+                else:
+                    created_at = created_at.astimezone(timezone.utc)
+                return created_at.timestamp()
+            return float('-inf')

Also applies to: 151-153

---

236-249: Align list output schema with describe: expose 'id' (keep 'config_id' as alias)

Keeps parity with describe JSON while preserving backward-compat callers.

 def _server_config_to_dict(config: MCPAppConfiguration) -> dict:
@@
-    return {
-        "config_id": config.appConfigurationId,
+    return {
+        "id": config.appConfigurationId,
+        "config_id": config.appConfigurationId,  # alias for backward-compat
         "app_id": config.app.appId if config.app else None,

---

290-291: Fix crash: createdAt may be None

Calling strftime on None raises AttributeError.

-        console.print(f"  Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}")
+        console.print(f"  Created: {(server.createdAt.strftime('%Y-%m-%d %H:%M:%S') if server.createdAt else 'Unknown')}")

src/mcp_agent/cli/cloud/commands/servers/utils.py (3)

39-51: Make format validation case-insensitive; accept 'yml' alias

Improves UX; CLI flags often vary in case and users type yml.

-def validate_output_format(format: str) -> None:
+def validate_output_format(format: str) -> None:
@@
-    valid_formats = ["text", "json", "yaml"]
-    if format not in valid_formats:
-        raise CLIError(f"Invalid format '{format}'. Valid options are: {', '.join(valid_formats)}")
+    valid_formats = {"text", "json", "yaml", "yml"}
+    fmt = (format or "").lower()
+    if fmt not in valid_formats:
+        raise CLIError(f"Invalid format '{format}'. Valid options are: text, json, yaml")

---

53-77: Simplify server resolution via API helper

You can delegate to the client’s get_app_or_config to reduce branching, keeping the same behavior for IDs and URLs.

-def resolve_server(client: MCPAppClient, id_or_url: str) -> Union[MCPApp, MCPAppConfiguration]:
+def resolve_server(client: MCPAppClient, id_or_url: str) -> Union[MCPApp, MCPAppConfiguration]:
@@
-    try:
-        app_id, config_id, server_url = parse_app_identifier(id_or_url)
-        
-        if server_url:
-            return run_async(client.get_app(server_url=server_url))
-        elif config_id:
-            return run_async(client.get_app_configuration(app_config_id=config_id))
-        else:
-            return run_async(client.get_app(app_id=app_id))
-            
+    try:
+        return run_async(client.get_app_or_config(id_or_url))

---

137-152: Optional: extend status mapping

If API adds statuses (e.g., STARTING, ERROR), consider mapping them for consistency in text/JSON.

src/mcp_agent/cli/cloud/commands/servers/list/main.py (2)

23-27: Avoid using built-ins as parameter names (filter, format)

Prevents shadowing and improves readability; CLI flags remain unchanged.

-def list_servers(
-    limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"),
-    filter: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"),
-    sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"),
-    format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"),
-) -> None:
+def list_servers(
+    limit: Optional[int] = typer.Option(None, "--limit", help="Maximum number of results to return"),
+    filter_param: Optional[str] = typer.Option(None, "--filter", help="Filter by name, description, or status (case-insensitive)"),
+    sort_by: Optional[str] = typer.Option(None, "--sort-by", help="Sort by field: name, created, status (prefix with - for reverse)"),
+    output_format: Optional[str] = typer.Option("text", "--format", help="Output format (text|json|yaml)"),
+) -> None:
@@
-    validate_output_format(format)
+    validate_output_format(output_format)
@@
-    filtered_deployed = _apply_filter(list_apps_res.apps, filter) if filter else list_apps_res.apps
-    filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter) if filter else list_app_configs_res.appConfigurations
+    filtered_deployed = _apply_filter(list_apps_res.apps, filter_param) if filter_param else list_apps_res.apps
+    filtered_configured = _apply_filter(list_app_configs_res.appConfigurations, filter_param) if filter_param else list_app_configs_res.appConfigurations
@@
-    if format == "json":
+    if output_format == "json":
         _print_servers_json(sorted_deployed, sorted_configured)
-    elif format == "yaml":
+    elif output_format == "yaml":
         _print_servers_yaml(sorted_deployed, sorted_configured)
     else:
-        _print_servers_text(sorted_deployed, sorted_configured, filter, sort_by)
+        _print_servers_text(sorted_deployed, sorted_configured, filter_param, sort_by)

---

46-48: Validate --limit (> 0) to prevent API misuse

Negative/zero values shouldn’t hit the API.

-    max_results = limit or 100
+    if limit is not None and limit <= 0:
+        raise typer.BadParameter("--limit must be a positive integer")
+    max_results = limit or 100

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/mcp_agent/cli/cloud/main.py (1)

154-156: Remove duplicate top-level 'apps' group alias (conflicts with existing apps group at Line 121)

This creates a Click/Typer collision. Keep the dedicated servers group, but drop the duplicate alias at the top level. The cloud-level alias (cloud apps) can remain.

 app_cmd_servers.command(name="delete")(delete_server)
 app.add_typer(app_cmd_servers, name="servers", help="Manage MCP Servers")
-
-# Alias for servers - apps should behave identically
-app.add_typer(app_cmd_servers, name="apps", help="Manage MCP Apps (alias for servers)")

src/mcp_agent/cli/cloud/commands/servers/list/main.py (3)

18-18: Make 'created' sorting robust across tz-naive/aware datetimes

Normalize to UTC and use epoch seconds; avoids TypeError and ensures consistent ordering.

-from datetime import datetime
+from datetime import datetime, timezone
@@
-        elif sort_field_lower in ['created', 'created_at', 'date']:
-            return created_at or datetime.min.replace(tzinfo=None if created_at is None else created_at.tzinfo)
+        elif sort_field_lower in ['created', 'created_at', 'date']:
+            if created_at:
+                if created_at.tzinfo is None:
+                    created_at = created_at.replace(tzinfo=timezone.utc)
+                else:
+                    created_at = created_at.astimezone(timezone.utc)
+                return created_at.timestamp()
+            return float('-inf')

Also applies to: 149-153

---

236-249: Align list JSON/YAML schema with describe output (id vs config_id)

Expose "id" for configurations (keep "config_id" as alias for BC).

 def _server_config_to_dict(config: MCPAppConfiguration) -> dict:
@@
-    return {
-        "config_id": config.appConfigurationId,
+    return {
+        "id": config.appConfigurationId,
+        "config_id": config.appConfigurationId,  # alias for backward-compat
         "app_id": config.app.appId if config.app else None,
         "name": config.app.name if config.app else "Unnamed",
         "description": config.app.description if config.app else None,
         "status": clean_server_status(status_raw),
         "server_url": config.appServerInfo.serverUrl if config.appServerInfo else None,
         "creator_id": config.creatorId,
         "created_at": config.createdAt.isoformat() if config.createdAt else None,
         "type": "configured"
     }

---

290-291: Guard createdAt and prefer ISO 8601 formatting

Avoid crashes when createdAt is None and keep consistent formatting across outputs.

-        console.print(f"  Created: {server.createdAt.strftime('%Y-%m-%d %H:%M:%S')}")
+        console.print(f"  Created: {(server.createdAt.isoformat() if server.createdAt else 'Unknown')}")
@@
-        if config.createdAt:
-            console.print(
-                f"  Created: {config.createdAt.strftime('%Y-%m-%d %H:%M:%S')}"
-            )
+        console.print(f"  Created: {(config.createdAt.isoformat() if config.createdAt else 'Unknown')}")

Also applies to: 322-325

src/mcp_agent/cli/cloud/commands/logger/tail/main.py (3)

21-22: Use run_async helper instead of asyncio.run for robustness

This avoids RuntimeError when invoked under an already-running event loop (e.g., tests) and aligns with servers/list usage.

-from mcp_agent.cli.core.utils import parse_app_identifier, resolve_server_url
+from mcp_agent.cli.core.utils import parse_app_identifier, resolve_server_url, run_async
@@
-            asyncio.run(_stream_logs(
+            run_async(_stream_logs(
                 app_id=app_id,
                 config_id=config_id,
                 server_url=server_url,
                 credentials=credentials,
                 grep_pattern=grep,
                 app_identifier=app_identifier,
                 format=format,
             ))
@@
-            asyncio.run(_fetch_logs(
+            run_async(_fetch_logs(
                 app_id=app_id,
                 config_id=config_id,
                 server_url=server_url,
                 credentials=credentials,
                 since=since,
                 grep_pattern=grep,
                 limit=limit,
                 order_by=order_by,
                 asc=asc,
                 desc=desc,
                 format=format,
             ))

Also applies to: 133-157

---

379-383: Simplify Rich style markup; remove unsupported “not bold”

Rich doesn’t support “not bold” in markup. Use dim for timestamps.

-    console.print(
-        f"[bright_black not bold]{timestamp}[/bright_black not bold] "
+    console.print(
+        f"[dim]{timestamp}[/dim] "
         f"[{level_style}]{level:7}[/{level_style}] "
         f"{message}"
     )

---

409-413: Stream YAML as a single mapping, not a singleton list

Better ergonomics for piping/jq/yq.

-    elif format == "yaml":
-        cleaned_entry = _clean_log_entry(log_entry)
-        print(yaml.dump([cleaned_entry], default_flow_style=False))
+    elif format == "yaml":
+        cleaned_entry = _clean_log_entry(log_entry)
+        print(yaml.dump(cleaned_entry, default_flow_style=False))

src/mcp_agent/cli/cloud/commands/servers/list/main.py (1)

314-316: Rename label to “App ID” for configured servers

For configurations, this value is the underlying App ID, not a “Server ID”.

-        if config.app:
-            console.print(f"  Server ID: {config.app.appId}")
+        if config.app:
+            console.print(f"  App ID: {config.app.appId}")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.